revert #16663, which was a revert of #16039 #16675

dgwynne · 2017-06-12T01:51:46Z

i believe the issue with #16039 was the loss of the semantics provided by the "b" part of the mode in fopen(). this is a nop on unix-like systems, but appears to be significant on windows.

the equivalent functionality on windows in open() is the O_BINARY flag. this adds that to the open() call, and provides compat for it on platforms without the flag by defining it to 0.

while here, fix an fd leak in new_file_source() if the buffer allocation fails.

closes #xxxx
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

This reverts commit c550372.

on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code.

gfyoung · 2017-06-12T03:20:04Z

You need to re-add the test that I wrote to ensure that your change is correct.

dgwynne · 2017-06-12T03:21:30Z

i did think it was going too well...

part of c550372

codecov · 2017-06-12T03:34:43Z

Codecov Report

Merging #16675 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16675      +/-   ##
==========================================
- Coverage   90.92%   90.92%   -0.01%     
==========================================
  Files         161      161              
  Lines       49272    49271       -1     
==========================================
- Hits        44803    44802       -1     
  Misses       4469     4469

Flag	Coverage Δ
#multiple	`88.69% <100%> (-0.01%)`	⬇️
#single	`40.22% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/io/parsers.py	`95.43% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0281886...0a831b6. Read the comment docs.

codecov · 2017-06-12T03:34:44Z

Codecov Report

Merging #16675 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16675      +/-   ##
==========================================
- Coverage   90.92%   90.92%   -0.01%     
==========================================
  Files         161      161              
  Lines       49272    49271       -1     
==========================================
- Hits        44803    44802       -1     
  Misses       4469     4469

Flag	Coverage Δ
#multiple	`88.69% <100%> (-0.01%)`	⬇️
#single	`40.22% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/io/parsers.py	`95.43% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0281886...0a831b6. Read the comment docs.

codecov · 2017-06-12T03:34:51Z

Codecov Report

Merging #16675 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16675      +/-   ##
==========================================
- Coverage   90.92%   90.92%   -0.01%     
==========================================
  Files         161      161              
  Lines       49272    49276       +4     
==========================================
+ Hits        44803    44804       +1     
- Misses       4469     4472       +3

Flag	Coverage Δ
#multiple	`88.68% <ø> (-0.01%)`	⬇️
#single	`40.18% <ø> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.23% <0%> (-0.1%)`	⬇️
pandas/plotting/_core.py	`81.83% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0281886...28ec409. Read the comment docs.

gfyoung · 2017-06-12T06:05:51Z

@dgwynne : LGTM, well done re-patching your patch 😄

dgwynne · 2017-06-12T06:09:12Z

@gfyoung : thanks. sorry for the breakage though. it never even occurred to me that there'd be encoding issues like this.

it's also encouraging to see how quickly such issues are handled. reverting the diff was obviously right at the time.

gfyoung · 2017-06-12T06:21:38Z

@dgwynne : No worries! We can't see everything that breaks if we don't test for it. ;)

jreback

approach lgtm. couple of comments.

jreback · 2017-06-12T10:44:53Z

doc/source/whatsnew/v0.20.3.txt

@@ -54,7 +54,6 @@ Indexing
 I/O
 ^^^

- Bug in ``pd.read_csv()`` in which files containing EOF characters mid-field could fail with the C engine on Windows (:issue:`16039`, :issue:`16559`)


re-add this (and add this PR number on as well)

jreback · 2017-06-12T10:45:08Z

pandas/_libs/src/parser/io.c

-    fs->initial_file_pos = ftell(fs->fp);
+    fs->fd = open(fname, O_RDONLY | O_BINARY);
+    if (fs->fd == -1) {
+        goto err_free;


I would in-line these instead of goto

im not keen on replacing the goto with duplicate code in all the error paths. in my experience using goto in this specific situation, ie, unwinding the generation of state like this, has more benefits than caveats. inlining the cleanup leads to larger code, and is more error prone to maintain. if you add the allocation of something else in the future, you have to judge and fix the cleanup in multiple places rather than adding to the stack of goto targets.

if you feel strongly about it ill change it. after all, im just visiting :). i just wanted to discuss why i did it like this in the first place.

i am fine either way

but right now it's about half and half

so if u can remove some calls and use the goto that's fine (or go the other way imho is actually more explicit)

code size makes almost no difference here
correct is the important issue

jreback · 2017-06-12T10:45:25Z

pandas/_libs/src/parser/io.c

+    if (mm->fd == -1) {
+        fprintf(stderr, "new_file_buffer: open(%s) failed. errno =%d\n",
+          fname, errno);
+        goto err_free;


jreback · 2017-06-12T10:45:29Z

pandas/_libs/src/parser/io.c

+    if (fstat(mm->fd, &stat) == -1) {
+        fprintf(stderr, "new_file_buffer: fstat() failed. errno =%d\n",
+          errno);
+        goto err_close;


jreback · 2017-06-12T10:45:34Z

pandas/_libs/src/parser/io.c

        /* XXX Eventually remove this print statement. */
        fprintf(stderr, "new_file_buffer: mmap() failed.\n");
-        free(mm);
-        mm = NULL;
+        goto err_close;


@jreback

requested by @jreback in #16675 feedback.

@jreback

requested by @jreback in feedback on #16675

jreback · 2017-06-13T10:20:59Z

thanks @dgwynne !

@jreback

…as-dev#16675) * Revert "BUG: Revert pandas-devgh-16039 (pandas-dev#16663)" This reverts commit c550372. * always treat files as binary to cope with windows and EOF. on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code. * dont leak the fd in new_file_source() if buffer allocation fails. * reapply the test for EOF in the middle of a stream. part of c550372 * pass rb to _get_handle on python 3, otherwise stick to r. part of c550372 * replace goto with inline unwinding of state. requested by @jreback in pandas-dev#16675 feedback. * describe the fixes to the read_csv() backend and issue numbers. requested by @jreback in feedback on pandas-dev#16675 (cherry picked from commit d298414)

@jreback

* Revert "BUG: Revert gh-16039 (#16663)" This reverts commit c550372. * always treat files as binary to cope with windows and EOF. on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code. * dont leak the fd in new_file_source() if buffer allocation fails. * reapply the test for EOF in the middle of a stream. part of c550372 * pass rb to _get_handle on python 3, otherwise stick to r. part of c550372 * replace goto with inline unwinding of state. requested by @jreback in #16675 feedback. * describe the fixes to the read_csv() backend and issue numbers. requested by @jreback in feedback on #16675 (cherry picked from commit d298414)

dgwynne added 3 commits June 12, 2017 11:35

Revert "BUG: Revert gh-16039 (#16663)"

b1d27a0

This reverts commit c550372.

dont leak the fd in new_file_source() if buffer allocation fails.

bad5795

dgwynne mentioned this pull request Jun 12, 2017

BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

Merged

gfyoung mentioned this pull request Jun 12, 2017

MAINT: Buttress memory handling in C csv engine #16673

Closed

dgwynne added 2 commits June 12, 2017 13:30

reapply the test for EOF in the middle of a stream.

28543ff

part of c550372

pass rb to _get_handle on python 3, otherwise stick to r.

0a831b6

part of c550372

jreback requested changes Jun 12, 2017

View reviewed changes

jreback added the IO CSV read_csv, to_csv label Jun 12, 2017

jreback added this to the 0.20.3 milestone Jun 12, 2017

jreback added the Needs Backport label Jun 12, 2017

dgwynne added 2 commits June 13, 2017 11:49

replace goto with inline unwinding of state.

48c5a0b

requested by @jreback in #16675 feedback.

describe the fixes to the read_csv() backend and issue numbers.

28ec409

requested by @jreback in feedback on #16675

jreback approved these changes Jun 13, 2017

View reviewed changes

jreback merged commit d298414 into pandas-dev:master Jun 13, 2017

TomAugspurger removed the Needs Backport label Jul 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert #16663, which was a revert of #16039 #16675

revert #16663, which was a revert of #16039 #16675

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

codecov bot commented Jun 12, 2017

codecov bot commented Jun 12, 2017

codecov bot commented Jun 12, 2017 •

edited

Loading

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

jreback left a comment

jreback Jun 12, 2017

dgwynne Jun 12, 2017

jreback Jun 12, 2017

dgwynne Jun 12, 2017

jreback Jun 12, 2017

dgwynne Jun 12, 2017

jreback Jun 13, 2017

jreback Jun 12, 2017

jreback Jun 12, 2017

jreback Jun 12, 2017

jreback commented Jun 13, 2017

revert #16663, which was a revert of #16039 #16675

revert #16663, which was a revert of #16039 #16675

Conversation

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

codecov bot commented Jun 12, 2017

Codecov Report

codecov bot commented Jun 12, 2017

Codecov Report

codecov bot commented Jun 12, 2017 • edited Loading

Codecov Report

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 13, 2017

codecov bot commented Jun 12, 2017 •

edited

Loading